Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support to filter traces using !~ #2410

Merged
merged 9 commits into from
May 2, 2023
Merged

Support to filter traces using !~ #2410

merged 9 commits into from
May 2, 2023

Conversation

kousikmitra
Copy link
Contributor

What this PR does:

  • Enables parsing !~ in tracql
  • Support for filtering values using negated regex pattern

Which issue(s) this PR fixes:
Fixes #1990

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

mdisibio
mdisibio reviewed May 1, 2023
View reviewed changes
Copy link
Contributor

@mdisibio mdisibio left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this. Overall looks great, just have a few small comments.

pkg/parquetquery/predicate_test.go Outdated Show resolved Hide resolved
pkg/parquetquery/predicate_test.go Outdated Show resolved Hide resolved
pkg/parquetquery/predicates.go Show resolved Hide resolved
@mdisibio
Copy link
Contributor

mdisibio commented May 1, 2023

There are some failing tests. I think they're all related and we need to remove the unsupported cases here :

    --- FAIL: TestExamples/unsupported_-_{_"test"_!~_"test"_} (0.00s)
    --- FAIL: TestExamples/unsupported_-_{_.a_!~_"test"_} (0.00s)

    --- FAIL: TestExamplesInEngine/unsupported_-_{_"test"_!~_"test"_} (0.00s)
    --- FAIL: TestExamplesInEngine/unsupported_-_{_.a_!~_"test"_} (0.00s)

=== RUN   TestExamples/unsupported_-_{_"test"_!~_"test"_}
    ast_validate_test.go:60: 
        	Error Trace:	/home/runner/work/tempo/tempo/pkg/traceql/ast_validate_test.go:60
        	Error:      	An error is expected but got nil.
        	Test:       	TestExamples/unsupported_-_{_"test"_!~_"test"_}
=== RUN   TestExamples/unsupported_-_{_.a_!~_"test"_}
    ast_validate_test.go:60: 
        	Error Trace:	/home/runner/work/tempo/tempo/pkg/traceql/ast_validate_test.go:60
        	Error:      	An error is expected but got nil.
        	Test:       	TestExamples/unsupported_-_{_.a_!~_"test"_}

@kousikmitra kousikmitra force-pushed the feature/not-regex-match branch from a5ac505 to 3b1c823 Compare May 1, 2023 14:35
@kousikmitra
Copy link
Contributor Author

e2e passed on my local, might be some flaky test

knylander-grafana
knylander-grafana reviewed May 1, 2023
View reviewed changes
Copy link
Contributor

@knylander-grafana knylander-grafana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding doc.

mdisibio
mdisibio approved these changes May 2, 2023
View reviewed changes
Copy link
Contributor

@mdisibio mdisibio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with the map[string] implementation as-is. There is no regression on current performance, so let's not hold up this awesome feature. We can follow up with performance improvements as needed.

@kousikmitra
Copy link
Contributor Author

I'm good with the map[string] implementation as-is. There is no regression on current performance, so let's not hold up this awesome feature. We can follow up with performance improvements as needed.

Sure, then we are good to go 🚀

@mdisibio mdisibio merged commit 6e7cd10 into grafana:main May 2, 2023
@kousikmitra kousikmitra deleted the feature/not-regex-match branch May 2, 2023 18:17
@joe-elliott joe-elliott mentioned this pull request May 8, 2023
Closed
@mdisibio mdisibio mentioned this pull request May 17, 2023
Merged
3 tasks
@joe-elliott joe-elliott mentioned this pull request May 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knylander-grafana knylander-grafana knylander-grafana left review comments

@mdisibio mdisibio mdisibio approved these changes

@joe-elliott joe-elliott Awaiting requested review from joe-elliott joe-elliott is a code owner

@annanay25 annanay25 Awaiting requested review from annanay25

@mapno mapno Awaiting requested review from mapno mapno is a code owner

@yvrhdn yvrhdn Awaiting requested review from yvrhdn yvrhdn is a code owner

@zalegrala zalegrala Awaiting requested review from zalegrala zalegrala is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[TraceQL] Support Not Regex (!~)
3 participants
@kousikmitra @mdisibio @knylander-grafana